Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return null instead of zero value uuid #2794

Merged
merged 1 commit into from
Sep 15, 2023

Conversation

0x221A
Copy link
Contributor

@0x221A 0x221A commented Sep 14, 2023

Describe your PR and link to any relevant issues.

Return null instead of the zero value uuid. I think this will be better and easier for the end user for null checking.

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@coveralls
Copy link

coveralls commented Sep 15, 2023

Coverage Status

coverage: 75.754% (+0.09%) from 75.661% when pulling 6d61287 on 0x221A:master into 625ca2e on 99designs:master.

@StevenACoffman StevenACoffman merged commit c89860b into 99designs:master Sep 15, 2023
17 checks passed
@StevenACoffman
Copy link
Collaborator

Thanks! I appreciate this. Looking forward to more good work from you in the future!

@it512
Copy link
Contributor

it512 commented Sep 15, 2023

no ... I think this is bug....

uuid is value type , like int.
default value is zero value ( uuid.Nil), in GraphQL is UUID!
nil ('null") is *uuid.UUID in Graphql is UUID. ( no !)

so this is bug!

@StevenACoffman @0x221A

@0x221A
Copy link
Contributor Author

0x221A commented Sep 15, 2023

@it512 Yeah, but I think nobody checking for zero value uuid. It's the same as time.Time that returns null instead of zero value time.

@it512 it512 mentioned this pull request Sep 15, 2023
@it512
Copy link
Contributor

it512 commented Sep 15, 2023

time.Time is struct , not a point, can not set to null!

*time.Time can set to nil !

time.Time is not *time.Time

@0x221A
Copy link
Contributor Author

0x221A commented Sep 15, 2023

For me, it still doesn't make sense to return zero value uuid. And you are right time.Time is struct but the marshaller still checking if it was zero time or not and still returns null if it was zero value time. If you dive into the generated code you will see it.

From what I see returning a zero value uuid is a bug unless you expected that and mostly no one expected the value 00000000-0000-0000-0000-000000000000 will return from the server instead of null.

@StevenACoffman
Copy link
Collaborator

There's always a problem translating between Go's "useful zero values" concept and GraphQL consumed by other languages.

  • The zero value in Go of an int is 0. gqlgen returns this in GraphQL as 0.
  • The zero value in Go of a time.Time is January 1, year 1, 00:00:00 UTC. gqlgen returns this in GraphQL as 'Null`.
  • The zero value in Go of a pointer to either of those types is nil. For nil pointers, gqlgen returns in GraphQL's Null.

You cannot assign a time.Time to nil in Go:

	var myTime time.Time
	myTime = nil // error: cannot use nil as time.Time value in assignment

You also cannot assign a uuid.UUID to nil in Go:

	var myUUID uuid.UUID
	myUUID = nil // error:  cannot use nil as uuid.UUID value in assignment

When a JavaScript client makes a GraphQL call using the GraphQL Basic scalar types of String, Int, Float, Boolean, and ID, we expect the response to be useful values. I think that an int value of 0 is actually quite likely to be useful, both in Go and in GraphQL.

This PR made it impossible for gqlgen to respond with the valid, but unlikely to be useful, UUID value of 00000000-0000-0000-0000-000000000000. This seems equivalent tow how it is impossible for gqlgen to respond with a valid, but unlikely to be useful, time of January 1, year 1, 00:00:00 UTC.

Can you explain how or where 00000000-0000-0000-0000-000000000000 in GraphQL would be a useful value rather than Null?

@a8m @giautm @x80486 I would appreciate your opinion on whether this PR should be reverted or not.

@x80486
Copy link

x80486 commented Sep 16, 2023

This is correct. Null values in GraphQL are represented by the keyword null. Consequently, 00000000-0000-0000-0000-000000000000 is only meaningful in the Go world, but when the GraphQL response is built, null is needed (as expected), rather than an invalid UUID to signify null.

Bottom line, in GraphQL, null is semantically correct — it represents the lack of a value. 00000000-0000-0000-0000-000000000000 does not.

@0x221A
Copy link
Contributor Author

0x221A commented Sep 16, 2023

I think we can assume that 00000000-0000-0000-0000-000000000000 should return null by taking a look at how google/uuid handles NULL value from the database.

@it512
Copy link
Contributor

it512 commented Sep 16, 2023

According to RFC 4122 Nil UUID is a valid UUID

https://datatracker.ietf.org/doc/html/rfc4122#section-4.1.7

It can usually be used as a placeholder in many scenarios, such as default values ​​of fields in databases, etc.

If Nil uuid == null, the Nil uuid field will return nil. . . this will be a disaster

If the UUID can be null, use a pointer type.

In graphql, you can use UUID( no !)

@0x221A @StevenACoffman @a8m

-------------- Chinese version

根据 RFC 4122 Nil UUID 本就是一种有效的UUID

https://datatracker.ietf.org/doc/html/rfc4122#section-4.1.7

通常在很多场景下都可以作为占位符使用,例如数据库中字段默认值等

如果 Nil uuid == null, Nil uuid 字段将返回 nil 。。。 这将是一个灾难

如果UUID 可以为空,请使用指针类型。

在graphql 中,你可以使用 UUID,没有!

@it512
Copy link
Contributor

it512 commented Sep 16, 2023

code in goole.uuid

https://github.com/google/uuid/blob/v1.3.1/sql.go#L15

I added some comments

// Scan implements sql.Scanner so UUIDs can be read from databases transparently.
// Currently, database types that map to string and []byte are supported. Please
// consult database-specific driver documentation for matching types.
func (uuid *UUID) Scan(src interface{}) error {
	switch src := src.(type) {
	case nil:     // case 1 if field is null such as varchar(36) null ...
		return nil // *UUID = nil (golang nil)

	case string:
		// if an empty UUID comes from a table, we return a null UUID
		if src == "" {
			return nil // case 2 empty string if field type is string and value is ""(empty string).  return uuid is nil (golang nil)
		}

		// see Parse for required string format
		u, err := Parse(src) // case 3 field has value if src is Nil uuid (128 bit 0) , uuid = Nil UUID, /////  ** #2794 return golang nil BUG!!! **
		if err != nil {
			return fmt.Errorf("Scan: %v", err)
		}

		*uuid = u

	case []byte:
		// if an empty UUID comes from a table, we return a null UUID
		if len(src) == 0 {
			return nil
		}

		// assumes a simple slice of bytes if 16 bytes
		// otherwise attempts to parse
		if len(src) != 16 {
			return uuid.Scan(string(src))
		}
		copy((*uuid)[:], src)

	default:
		return fmt.Errorf("Scan: unable to scan type %T into UUID", src)
	}

	return nil
}

@0x221A @StevenACoffman

@0x221A
Copy link
Contributor Author

0x221A commented Sep 16, 2023

I think returning null first is the best way unless the user knows what they're dealing with. If that is the case use custom mapping.

And for me returning a zero value uuid without a clue will be a disaster more than null.

@it512
Copy link
Contributor

it512 commented Sep 16, 2023

If I want to return Nil UUID how can I do it? no way in #2794 !!!

In fact, any language distinguishes between zero values ​​and null values. but golang has made clear regulations on this

such as java, int and Integer
in java int called native type, default is 0, Integer called wapper type (Object type) default is null (java has no pointer)

in database if using int , null int field was return 0 (int default value), return null if using Integer

UUID in gqlgen is usedef type,Implemented according to RFC4122, base on google.uuid
it has zero value (Nil), in rfc 4122 defined Nil type UUID, google.uuid implement it

I think returning null according to usage, UUID! return zero value, UUID (no !) is *UUID default return nil (golang nil)

@0x221A
Copy link
Contributor Author

0x221A commented Sep 16, 2023

@StevenACoffman I'll leave this to you because it can go both ways. Because the implementation of json.Unmarshaler treats both null and zero value uuid the same way. And I found that google/uuid has a NullUUID type that can handle both ways. But I think it's not convenient to use it.

@it512
Copy link
Contributor

it512 commented Sep 16, 2023

The behavior of json.Unmarshaler depends on the type. If it is a pointer type, null will be used. In other cases, zero value will be used.

@0x221A
Copy link
Contributor Author

0x221A commented Sep 16, 2023

Yes, that is the default. But for the NullUUID type, it has an additional Valid field that indicates whether to use a null or zero value...

@it512
Copy link
Contributor

it512 commented Sep 16, 2023

For valid uuid NullUUID returns the correct UUID (valid is true), Nil (zero uuid) is valid, so the return is still Nil

NullUUID should only be used in ambiguous scenarios, or as a simple tool

graphql has a definition for nullability, use (!)

If available, please use UUID (no!)

github-merge-queue bot referenced this pull request in infratographer/x Sep 28, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [github.com/99designs/gqlgen](https://togithub.com/99designs/gqlgen) |
require | patch | `v0.17.36` -> `v0.17.38` |

---

### Release Notes

<details>
<summary>99designs/gqlgen (github.com/99designs/gqlgen)</summary>

###
[`v0.17.38`](https://togithub.com/99designs/gqlgen/releases/tag/v0.17.38)

[Compare
Source](https://togithub.com/99designs/gqlgen/compare/v0.17.37...v0.17.38)

#### What's Changed

- Ability to use forceGenerate and extraFields together by
[@&#8203;atzedus](https://togithub.com/atzedus) in
[https://github.com/99designs/gqlgen/pull/2788](https://togithub.com/99designs/gqlgen/pull/2788)
- Add new changelog by
[@&#8203;StevenACoffman](https://togithub.com/StevenACoffman) in
[https://github.com/99designs/gqlgen/pull/2787](https://togithub.com/99designs/gqlgen/pull/2787)
- Fix rand int docs link in Getting Started by
[@&#8203;stevenschobert](https://togithub.com/stevenschobert) in
[https://github.com/99designs/gqlgen/pull/2789](https://togithub.com/99designs/gqlgen/pull/2789)
- Make it possible to pass UI headers to GraphiQL by
[@&#8203;marcusthelin](https://togithub.com/marcusthelin) in
[https://github.com/99designs/gqlgen/pull/2793](https://togithub.com/99designs/gqlgen/pull/2793)
- Return `null` instead of zero value uuid by
[@&#8203;0x221A](https://togithub.com/0x221A) in
[https://github.com/99designs/gqlgen/pull/2794](https://togithub.com/99designs/gqlgen/pull/2794)
- Update gqlparser to 2.5.10 by
[@&#8203;StevenACoffman](https://togithub.com/StevenACoffman) in
[https://github.com/99designs/gqlgen/pull/2798](https://togithub.com/99designs/gqlgen/pull/2798)

#### New Contributors

- [@&#8203;stevenschobert](https://togithub.com/stevenschobert) made
their first contribution in
[https://github.com/99designs/gqlgen/pull/2789](https://togithub.com/99designs/gqlgen/pull/2789)
- [@&#8203;marcusthelin](https://togithub.com/marcusthelin) made their
first contribution in
[https://github.com/99designs/gqlgen/pull/2793](https://togithub.com/99designs/gqlgen/pull/2793)
- [@&#8203;0x221A](https://togithub.com/0x221A) made their first
contribution in
[https://github.com/99designs/gqlgen/pull/2794](https://togithub.com/99designs/gqlgen/pull/2794)

**Full Changelog**:
99designs/gqlgen@v0.17.37...v0.17.38

###
[`v0.17.37`](https://togithub.com/99designs/gqlgen/blob/HEAD/CHANGELOG.md#v01737---2023-09-08)

[Compare
Source](https://togithub.com/99designs/gqlgen/compare/v0.17.36...v0.17.37)

- <a
href="https://togithub.com/99designs/gqlgen/commit/ccae370e96cbca6ce5deaabf28a6d57e3b181b3b"><tt>[`ccae370`](https://togithub.com/99designs/gqlgen/commit/ccae370e)</tt></a>
release v0.17.37

- <a
href="https://togithub.com/99designs/gqlgen/commit/6505f8be0d99593376d6c9a0dea00af5e3c018ea"><tt>[`6505f8b`](https://togithub.com/99designs/gqlgen/commit/6505f8be)</tt></a>
Update gqlparser (<a
href="https://togithub.com/99designs/gqlgen/pull/2785">[#&#8203;2785](https://togithub.com/99designs/gqlgen/issues/2785)</a>)

<dl><dd><details><summary><a
href="https://togithub.com/99designs/gqlgen/commit/153ec470d993a39c1656fc45432740f1d3dd10ea"><tt>153ec470</tt></a>
add uuid type (<a
href="https://togithub.com/99designs/gqlgen/pull/2751">#&#8203;2751</a>)
(closes <a href="https://togithub.com/99designs/gqlgen/issues/2749">
#&#8203;2749</a>)</summary>

-   add uuid type

-   add uuid example

-   add uuid scalar doc

-   strconv.Quote

-   Apply suggestions from code review

-   fix

***

</details></dd></dl>

<dl><dd><details><summary><a
href="https://togithub.com/99designs/gqlgen/commit/fa4711801c59d27db887a50e1f8393006268194e"><tt>fa471180</tt></a>
ForceGenerate parameter to
[@&#8203;goModel](https://togithub.com/goModel) added. (<a
href="https://togithub.com/99designs/gqlgen/pull/2780">#&#8203;2780</a>)</summary>

-   forceGenerate to docs added

***

</details></dd></dl>

<dl><dd><details><summary><a
href="https://togithub.com/99designs/gqlgen/commit/11bb9b1890fd9e5909f06feba7b2985bf09785c9"><tt>11bb9b18</tt></a>
codegen: add support for `go_build_tags` option in gqlgen.yaml (<a
href="https://togithub.com/99designs/gqlgen/pull/2784">#&#8203;2784</a>)</summary>

-   codegen: support go_build_tags option in gqlgen.yaml

-   chore: added test

-   docs/content: update config example

-   chore: more comment

</details></dd></dl>

<dl><dd><details><summary><a
href="https://togithub.com/99designs/gqlgen/commit/bee47dcf1f3edb95e7a77e612ea27d85a417b12c"><tt>bee47dcf</tt></a>
fix flaky test TestSubscriptions (<a
href="https://togithub.com/99designs/gqlgen/pull/2779">#&#8203;2779</a>)</summary>

-   fix flaky test TestSubscriptions

-   update other copy of the test

</details></dd></dl>

- <a
href="https://togithub.com/99designs/gqlgen/commit/a41f4daad66cb0c102bfa09d7bff5a057d378197"><tt>[`a41f4da`](https://togithub.com/99designs/gqlgen/commit/a41f4daa)</tt></a>
docs: short-lived loader (<a
href="https://togithub.com/99designs/gqlgen/pull/2778">[#&#8203;2778](https://togithub.com/99designs/gqlgen/issues/2778)</a>)

- <a
href="https://togithub.com/99designs/gqlgen/commit/cc4e0ba28375e0ec39c586485fa138c28e5bfcba"><tt>[`cc4e0ba`](https://togithub.com/99designs/gqlgen/commit/cc4e0ba2)</tt></a>
ensure HasOperationContext checks for nil (<a
href="https://togithub.com/99designs/gqlgen/pull/2776">[#&#8203;2776](https://togithub.com/99designs/gqlgen/issues/2776)</a>)

<dl><dd><details><summary><a
href="https://togithub.com/99designs/gqlgen/commit/a1ca220477092398efc8e587dd653f5cb967d0e9"><tt>a1ca2204</tt></a>
fix typo in TESTING.md server path (<a
href="https://togithub.com/99designs/gqlgen/pull/2774">#&#8203;2774</a>)</summary>

following TESTING.md instructions, I got an error:
"stat ./server/server.go: no such file or directory"

server.go path is: integration/server/cmd/integration/server.go

</details></dd></dl>

<dl><dd><details><summary><a
href="https://togithub.com/99designs/gqlgen/commit/1cde8c3fab65847a6e8d7f423215b786b47c19df"><tt>1cde8c3f</tt></a>
return internal types in schema introspection (<a
href="https://togithub.com/99designs/gqlgen/pull/2773">#&#8203;2773</a>)</summary>

according to graphql spec:

types: return the set of all named types contained within this schema.
Any named type which can be found through a field of any introspection
type must be included in this set.

source:
https://github.com/graphql/graphql-spec/blob/main/spec/Section%204%20--%20Introspection.md#the-\__schema-type

some clients libs (like HotChocolate for C#) depends on this behavior.

</details></dd></dl>

- <a
href="https://togithub.com/99designs/gqlgen/commit/065aea3efa27f0662eb2f62ef2dd131e153009ba"><tt>[`065aea3`](https://togithub.com/99designs/gqlgen/commit/065aea3e)</tt></a>
Fix gqlgen truncates tag value with colon (<a
href="https://togithub.com/99designs/gqlgen/pull/2759">[#&#8203;2759](https://togithub.com/99designs/gqlgen/issues/2759)</a>)

- <a
href="https://togithub.com/99designs/gqlgen/commit/d6270e4f4fd951c71ee2a2b997d7098b04e07976"><tt>[`d6270e4`](https://togithub.com/99designs/gqlgen/commit/d6270e4f)</tt></a>
Update subsciptions documentation to correctly close channel (<a
href="https://togithub.com/99designs/gqlgen/pull/2753">[#&#8203;2753](https://togithub.com/99designs/gqlgen/issues/2753)</a>)

- <a
href="https://togithub.com/99designs/gqlgen/commit/2d8673a691deffe6ddd1f4d5f013a52dc91aef91"><tt>[`2d8673a`](https://togithub.com/99designs/gqlgen/commit/2d8673a6)</tt></a>
Add Model references to Interface (<a
href="https://togithub.com/99designs/gqlgen/pull/2738">[#&#8203;2738](https://togithub.com/99designs/gqlgen/issues/2738)</a>)

- <a
href="https://togithub.com/99designs/gqlgen/commit/790d7a7571865b8b8324557e1a565f40c23217c8"><tt>[`790d7a7`](https://togithub.com/99designs/gqlgen/commit/790d7a75)</tt></a>
Allow GraphiQL headers to be set when creating the playground handler
(<a
href="https://togithub.com/99designs/gqlgen/pull/2740">[#&#8203;2740](https://togithub.com/99designs/gqlgen/issues/2740)</a>)
(closes <a href="https://togithub.com/99designs/gqlgen/issues/2739">
[#&#8203;2739](https://togithub.com/99designs/gqlgen/issues/2739)</a>)

- <a
href="https://togithub.com/99designs/gqlgen/commit/0eb95dc4315fc5f74431df03e008283cf9ec0c35"><tt>[`0eb95dc`](https://togithub.com/99designs/gqlgen/commit/0eb95dc4)</tt></a>
v0.17.36 postrelease bump

 <!-- end of Commits -->

<!-- end of Else -->

<!-- end of If NoteGroups -->

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/infratographer/x).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi44My4wIiwidXBkYXRlZEluVmVyIjoiMzYuOTcuMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants